fix: add IgnoreUnexported to Node Taints comparison #1371

afarbos · 2024-11-18T17:40:01Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The goal of this PR is to fix a panic after creating a cluster around node taint:

panic: cannot handle unexported field at {[]*containerpb.NodeTaint}[0].state:
"cloud.google.com/go/container/apiv1/containerpb".NodeTaint
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]
	panic: cannot handle unexported field at {[]*containerpb.NodeTaint}[0].state:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Full error and stack trace bellow:

panic: cannot handle unexported field at {[]*containerpb.NodeTaint}[0].state:
	"cloud.google.com/go/container/apiv1/containerpb".NodeTaint
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]
	panic: cannot handle unexported field at {[]*containerpb.NodeTaint}[0].state:
	"cloud.google.com/go/container/apiv1/containerpb".NodeTaint
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported

goroutine 90 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:111 +0x1e5
panic({0x2d817a0?, 0xc0021251a0?})
	runtime/panic.go:770 +0x132
github.com/google/go-cmp/cmp.validator.apply({{}}, 0xc001284dc0, {0x335a9e0?, 0xc0005bf630?, 0xc002125100?}, {0x335a9e0?, 0xc001544b90?, 0x335a9e0?})
	github.com/google/[email protected]/cmp/options.go:248 +0x3ab
github.com/google/go-cmp/cmp.(*state).tryOptions(0xc001284dc0, {0x3d13170, 0x335a9e0}, {0x335a9e0?, 0xc0005bf630?, 0x10?}, {0x335a9e0?, 0xc001544b90?, 0xc000680008?})
	github.com/google/[email protected]/cmp/compare.go:306 +0x109
github.com/google/go-cmp/cmp.(*state).compareAny(0xc001284dc0, {0x3ceca30, 0xc0020a2800})
	github.com/google/[email protected]/cmp/compare.go:261 +0x4c7
github.com/google/go-cmp/cmp.(*state).compareStruct(0xc001284dc0, {0x3d13170, 0x34a8300}, {0x34a8300?, 0xc0005bf630?, 0x4182df?}, {0x34a8300?, 0xc001544b90?, 0x407f4b?})
	github.com/google/[email protected]/cmp/compare.go:414 +0x586
github.com/google/go-cmp/cmp.(*state).compareAny(0xc001284dc0, {0x3cecac0, 0xc00212e680})
	github.com/google/[email protected]/cmp/compare.go:289 +0xcce
github.com/google/go-cmp/cmp.(*state).comparePtr(0xc001284dc0, {0x3d13170, 0x3367960}, {0x3367960?, 0xc000272d48?, 0xc00212e640?}, {0x3367960?, 0xc00138e100?, 0xc0011edc08?})
	github.com/google/[email protected]/cmp/compare.go:566 +0x373
github.com/google/go-cmp/cmp.(*state).compareAny(0xc001284dc0, {0x3ceca60, 0xc000383e60})
	github.com/google/[email protected]/cmp/compare.go:295 +0xb72
github.com/google/go-cmp/cmp.(*state).statelessCompare(0xc001284dc0, {0x3ceca60?, 0xc000383e60?})
	github.com/google/[email protected]/cmp/compare.go:232 +0x7a
github.com/google/go-cmp/cmp.(*state).compareSlice.func2(0x0?, 0xc000383f80?)
	github.com/google/[email protected]/cmp/compare.go:480 +0x5a
github.com/google/go-cmp/cmp/internal/diff.Difference(0x1, 0x1, 0xc0011ee038)
	github.com/google/[email protected]/cmp/internal/diff/diff.go:286 +0x3e2
github.com/google/go-cmp/cmp.(*state).compareSlice(0xc001284dc0, {0x3d13170, 0x2d112c0}, {0x2d112c0?, 0xc000f9ae28?, 0xc002125f80?}, {0x2d112c0?, 0xc000f9ae40?, 0x30?})
	github.com/google/[email protected]/cmp/compare.go:479 +0x9ac
github.com/google/go-cmp/cmp.(*state).compareAny(0xc001284dc0, {0x3cecb50, 0xc00212e600})
	github.com/google/[email protected]/cmp/compare.go:291 +0xbbb
github.com/google/go-cmp/cmp.Equal({0x2d112c0, 0xc000f9ae28}, {0x2d112c0, 0xc000f9ae40}, {0x0?, 0x2b?, 0x0?})
	github.com/google/[email protected]/cmp/compare.go:97 +0x65
sigs.k8s.io/cluster-api-provider-gcp/cloud/services/container/nodepools.(*Service).checkDiffAndPrepareUpdateConfig(0xc0009182c0, 0xc0011c48c0)
	sigs.k8s.io/cluster-api-provider-gcp/cloud/services/container/nodepools/reconcile.go:373 +0x555
sigs.k8s.io/cluster-api-provider-gcp/cloud/services/container/nodepools.(*Service).Reconcile(0xc0009182c0, {0x3cef5e8, 0xc00003c000})
	sigs.k8s.io/cluster-api-provider-gcp/cloud/services/container/nodepools/reconcile.go:147 +0xfa5
sigs.k8s.io/cluster-api-provider-gcp/exp/controllers.(*GCPManagedMachinePoolReconciler).reconcile(0xc001066078?, {0x3cef5e8, 0xc00003c000}, 0xc000927d10)
	sigs.k8s.io/cluster-api-provider-gcp/exp/controllers/gcpmanagedmachinepool_controller.go:334 +0x4de
sigs.k8s.io/cluster-api-provider-gcp/exp/controllers.(*GCPManagedMachinePoolReconciler).Reconcile(0xc000d98b40, {0x3cef540?, 0xc000e7ba70?}, {{{0xc000e00b60?, 0x0?}, {0xc0003af650?, 0xc000b39d10?}}})
	sigs.k8s.io/cluster-api-provider-gcp/exp/controllers/gcpmanagedmachinepool_controller.go:316 +0x8df
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0x3cf66a0?, {0x3cef540?, 0xc000e7ba70?}, {{{0xc000e00b60?, 0xb?}, {0xc0003af650?, 0x0?}}})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:114 +0xb7
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc000674210, {0x3cef578, 0xc000936c30}, {0x3188560, 0xc000b48820})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:311 +0x3bc
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc000674210, {0x3cef578, 0xc000936c30})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:261 +0x1be
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:222 +0x79
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2 in goroutine 220
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:218 +0x486

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

fix panic when setting node taints

k8s-ci-robot · 2024-11-18T17:40:11Z

Hi @afarbos. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2024-11-18T17:40:19Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

Name	Link
🔨 Latest commit	`92abefc`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-cluster-api-gcp/deploys/688a3f1ddfbf69000862199e
😎 Deploy Preview	https://deploy-preview-1371--kubernetes-sigs-cluster-api-gcp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

afarbos · 2024-11-18T17:55:37Z

I am not clear, how to add a test in repo. Let me know if what I did is wrong.

cpanato · 2024-11-19T08:44:47Z

/ok-to-test

salasberryfin · 2024-11-21T18:06:54Z

Thanks @afarbos. Overall this looks good to me (just added a minor comment) and the template you modified should be enough to update the E2E test for GKE provisioning but, in this case, I see the specific GKE test cases are skipped (you can take a look at it in the prow dashboard).

@cpanato @damdo @richardcase are these E2E tests skipped because there are no changes in the managed cluster code (in ./exp/)? In this case it would be nice to also validate GKE's functionality.

test/e2e/data/infrastructure-gcp/cluster-template-ci-gke.yaml

damdo · 2024-12-16T09:31:51Z

/assign @cpanato @richardcase

salasberryfin · 2025-01-02T09:23:43Z

Hey @afarbos, thanks for this. I see the GKE tests are now running and seem to be failing, could you take a look at this?

afarbos · 2025-01-03T20:05:01Z

Hey @afarbos, thanks for this. I see the GKE tests are now running and seem to be failing, could you take a look at this?

Hello @salasberryfin , the issue seems to be linked with missing permissions on the GCP service account, see the error

E1220 17:12:48.570475       1 kubeconfig.go:149] "failed generating token" err=<
	error generating access token: rpc error: code = PermissionDenied desc = Permission 'iam.serviceAccounts.getAccessToken' denied on resource (or it may not exist).
	error details: name = ErrorInfo reason = IAM_PERMISSION_DENIED domain = iam.googleapis.com metadata = map[permission:iam.serviceAccounts.getAccessToken]

in the logs results (https://gcsweb.k8s.io/gcs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-gcp/1371/pull-cluster-api-provider-gcp-e2e-test/1870150705805791232/artifacts/clusters/bootstrap/logs/capg-system/capg-controller-manager/capg-controller-manager-67ccf7db6d-6hw2w/)

I am not sure where the service account is setup, I could not find it in the test here https://github.com/kubernetes-sigs/cluster-api-provider-gcp/blob/main/test/e2e/e2e_gke_test.go.

If it's setup manually, could you add roles/iam.serviceAccountTokenCreator to the service account? See documentation https://cloud.google.com/iam/docs/understanding-roles#iam.serviceAccountTokenCreator

afarbos · 2025-01-13T17:36:55Z

relates to kubernetes/k8s.io#7665

k8s-triage-robot · 2025-04-13T18:25:35Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

damdo · 2025-05-19T08:56:56Z

Opened #1476 to try and fix the version issue

k8s-triage-robot · 2025-06-18T09:06:22Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

afarbos · 2025-06-18T16:11:01Z

/remove-lifecycle rotten
Waiting for the test this panic is still here

damdo · 2025-06-20T06:51:34Z

@afarbos The test fix has been superseded by #1487

afarbos · 2025-06-25T15:20:09Z

@afarbos The test fix has been superseded by #1487

nice, rebasing and only including the fix!

salasberryfin · 2025-07-02T06:43:42Z

/retest

salasberryfin

Thanks @afarbos. I left one comment to understand the proposed changes.

test/e2e/data/infrastructure-gcp/cluster-template-ci-gke.yaml

salasberryfin · 2025-07-30T08:53:45Z

Thanks @afarbos, would you be able to rebase this? The problem with gcp-apidiff should now be fixed in main.

afarbos · 2025-07-30T15:50:20Z

Thanks @afarbos, would you be able to rebase this? The problem with gcp-apidiff should now be fixed in main.

rebased

afarbos · 2025-07-30T18:02:21Z

/retest

afarbos · 2025-07-30T18:02:45Z

/retest

flaky test? since I did not change anything related to MachineDeployment.

afarbos · 2025-07-30T18:47:44Z

👏 no more status check errors! 🤞 it's good now

salasberryfin · 2025-07-31T13:48:59Z

Thanks!

/lgtm

damdo

/approve

Thanks!

k8s-ci-robot · 2025-08-05T14:42:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afarbos, damdo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [damdo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from damdo and salasberryfin November 18, 2024 17:40

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 18, 2024

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 18, 2024

afarbos marked this pull request as ready for review November 18, 2024 17:55

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024

k8s-ci-robot requested a review from richardcase November 18, 2024 17:55

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2024

salasberryfin reviewed Nov 21, 2024

View reviewed changes

test/e2e/data/infrastructure-gcp/cluster-template-ci-gke.yaml Outdated Show resolved Hide resolved

k8s-ci-robot assigned cpanato and richardcase Dec 16, 2024

afarbos force-pushed the af/fixNodeTaintPanic branch from 2e4af5c to ef0b820 Compare December 20, 2024 16:53

afarbos mentioned this pull request Jan 16, 2025

Patch release v1.8.1 #1393

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 13, 2025

afarbos mentioned this pull request May 8, 2025

feat: add support for identity service server and updating identity service #1385

Open

3 tasks

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 18, 2025

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jun 18, 2025

afarbos force-pushed the af/fixNodeTaintPanic branch from 55ddf75 to 9044ac3 Compare June 25, 2025 15:19

afarbos force-pushed the af/fixNodeTaintPanic branch from 9044ac3 to e5a4e0e Compare July 11, 2025 21:25

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 11, 2025

afarbos force-pushed the af/fixNodeTaintPanic branch from e5a4e0e to a0ea025 Compare July 14, 2025 19:03

salasberryfin reviewed Jul 16, 2025

View reviewed changes

test/e2e/data/infrastructure-gcp/cluster-template-ci-gke.yaml Show resolved Hide resolved

afarbos requested a review from salasberryfin July 17, 2025 15:58

fix: add IgnoreUnexported to Node Taints comparison

92abefc

afarbos force-pushed the af/fixNodeTaintPanic branch from a0ea025 to 92abefc Compare July 30, 2025 15:49

k8s-ci-robot assigned salasberryfin Jul 31, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2025

damdo reviewed Aug 5, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2025

k8s-ci-robot merged commit 9eed3ec into kubernetes-sigs:main Aug 5, 2025
17 checks passed

fix: add IgnoreUnexported to Node Taints comparison #1371

fix: add IgnoreUnexported to Node Taints comparison #1371

Uh oh!

Conversation

afarbos commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 18, 2024

Uh oh!

netlify bot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

Uh oh!

afarbos commented Nov 18, 2024

Uh oh!

cpanato commented Nov 19, 2024

Uh oh!

salasberryfin commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

damdo commented Dec 16, 2024

Uh oh!

salasberryfin commented Jan 2, 2025

Uh oh!

afarbos commented Jan 3, 2025

Uh oh!

afarbos commented Jan 13, 2025

Uh oh!

k8s-triage-robot commented Apr 13, 2025

Uh oh!

damdo commented May 19, 2025

Uh oh!

k8s-triage-robot commented Jun 18, 2025

Uh oh!

afarbos commented Jun 18, 2025

Uh oh!

damdo commented Jun 20, 2025

Uh oh!

afarbos commented Jun 25, 2025

Uh oh!

salasberryfin commented Jul 2, 2025

Uh oh!

salasberryfin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

salasberryfin commented Jul 30, 2025

Uh oh!

afarbos commented Jul 30, 2025

Uh oh!

afarbos commented Jul 30, 2025

Uh oh!

afarbos commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afarbos commented Jul 30, 2025

Uh oh!

salasberryfin commented Jul 31, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

afarbos commented Nov 18, 2024 •

edited

Loading

netlify bot commented Nov 18, 2024 •

edited

Loading

salasberryfin commented Nov 21, 2024 •

edited

Loading

afarbos commented Jul 30, 2025 •

edited

Loading